feat(forge): revert diagnostic inspector #10446

0xrusowsky · 2025-05-06T12:00:08Z

closes

Better error reporting when interacting with undeployed contracts #4141

design

Created a new RevertDiagnostic inspector that provides detailed information of otherwise undefined evm reverts. For the time being, the inspector suports:

#[derive(Debug, Clone, Copy)]
pub enum DetailedRevertReason {
    CallToNonContract(Address),
    DelegateCallToNonContract(Address),
}

Additionally, enhanced the traces decoder to determine when a function call to an unkown selector takes places (only for local contracts with artifacts).

output

these would be the new error messages that users would see:

Traces:
  [6350] NonContractCallRevertTest::test_non_contract_call_failure()
    ├─ [0] console::log("test non contract call failure") [staticcall]
    │   └─ ← [Stop]
    ├─ [0] 0xdEADBEeF00000000000000000000000000000000::number()
    │   └─ ← [Stop]
    └─ ← [Revert] call to non-contract address 0xdEADBEeF00000000000000000000000000000000
    // rest of the logs

Traces:
  [255303] NonContractDelegateCallRevertTest::test_unlinked_library_call_failure()
    ├─ [0] console::log("Test: Simulating call to unlinked library") [staticcall]
    │   └─ ← [Stop]
    ├─ [214746] → new LibraryCaller@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← [Return] 960 bytes of code
    ├─ [3896] LibraryCaller::foobar(10)
    │   ├─ [0] 0xdEADBEeF00000000000000000000000000000000::foo(10) [delegatecall]
    │   │   └─ ← [Stop]
    │   └─ ← [Revert] delegatecall to non-contract address 0xdEADBEeF00000000000000000000000000000000 (usually an unliked library)
    // rest of the logs

Traces:
  [8620] NonContractCallRevertTest::test_non_supported_selector_call_failure()
    ├─ [0] console::log("test non supported fn selector call failure") [staticcall]
    │   └─ ← [Stop]
    ├─ [145] Counter::random()
    │   └─ ← [Revert] unrecognized function selector 0x5ec01e4d for contract 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, which has no fallback function.
    // rest of the logs

benchmarks `forge test -vvvv`

Project	Before [v1.2.1]	After	Overhead
morpho-blue	1.93s	2.14s	10.8%
spark-psm	39.56s	44.88s	13.4%
uniswap/v4-core	6.93s	7.72s	11.4%
vectorized/solady	2.44s	2.67s	9.4%

notes:

benchmarks only measure running of tests and are pre-build
benchmarks were run on an m2pro with 16gb of RAM
each test run 5 times and the reported time is the avg

conclusion: the new inspector makes tests (on average) ~11% slower... the bright side is that it would only impact those test runs where traces are active (which i would assume to only be when actively debugging).

do we feel like this is acceptable?

grandizzy

IMO on the right track, @klkvr pls share your thoughts re the approach.
left some minor comments / nits.
To note that such default option could add perf penalty for invariant tests running with fail_on_revert = false but probably bearable.

crates/evm/evm/src/inspectors/revert_diagnostic.rs

crates/evm/evm/src/inspectors/stack.rs

crates/forge/tests/cli/test_cmd.rs

grandizzy · 2025-05-07T08:31:31Z

crates/evm/evm/src/inspectors/revert_diagnostic.rs

+        outcome: CallOutcome,
+    ) -> CallOutcome {
+        if outcome.result.result == InstructionResult::Revert {
+            self.reverted = true


maybe here we could directly set the reason? we can also do more checks if target address is actually a contract, (like checking if selector available, etc. some other similar hh checks https://github.com/NomicFoundation/hardhat/blob/67f1e95e1f3904f7b2e8a5560115c1551e899f64/packages/hardhat-core/src/internal/hardhat-network/stack-traces/solidity-errors.ts#L218-L341)

i thought that storing a bool could be more efficient + fn reason() would allow us to isolate the logic, but totally fine for me to do it directly

I think we need some smarter strategy here to ensure that call we've caught in call hook is indeed the one that caused revert (e.g compare depth, ensure there were no calls after it)

klkvr · 2025-05-14T14:48:22Z

crates/forge/tests/cli/test_cmd.rs

+interface ICounter {
+    function number() external returns (uint256);
+}


It seems that in cases of calling functions returning data Solidity's approach is to revert if data is empty.

However, if method does not return anything (e.g like increment()) then Solidity would firstly check before the call whether the address has any code, and revert if it's not which I believe is not being caught by our inspector rn

0xrusowsky · 2025-05-15T15:07:28Z

addresses the PR feedback by checking the call stack depth + tracking EXTCODESIZE calls to empty addresses (confirmed that the initial impl wouldn't diagnose those).

the only downside is extra performance overhead, but i tried to make the fn setp as efficient as possible (improvement suggestions are welcome)

0xrusowsky · 2025-05-16T18:38:28Z

i also enhanced the trace decoder to identify function calls for a selector that is not supported by the called contract (if the tracer has access to its abi)

crates/evm/evm/src/inspectors/revert_diagnostic.rs

grandizzy

lgtm! one comment re enabling diagnostics and tracing, pls check

crates/forge/src/multi_runner.rs

grandizzy

makes sense, thank you! pending others review before merging
CC @klkvr

zerosnacks

lgtm 👍 pending final review from @klkvr

Note: there will likely be a small merge conflict after I merge #10183

0xrusowsky · 2025-05-19T09:57:58Z

lgtm 👍 pending final review from @klkvr

Note: there will likely be a small merge conflict after I merge #10183

great, thanks!
np, i'll take care of that once it shows up 🤝

klkvr

very nice! overall the approach lgtm, this feature is kind of tricky because we're basically forced to do bytecode/execution pattern matching and I'd like us to reduce false positives here and make sure we document this clearly because this logic will likely get more complex over time

crates/evm/evm/src/inspectors/revert_diagnostic.rs

klkvr · 2025-05-20T02:18:40Z

crates/evm/traces/src/decoder/mod.rs

+            // Check if unsupported fn selector: calldata dooes NOT point to one of its selectors +
+            // non-fallback contract + no receive
+            if let Some(contract_selectors) = self.non_fallback_contracts.get(&trace.address) {
+                if !contract_selectors.contains(&selector) &&
+                    (!cdata.is_empty() || !self.receive_contracts.contains(&trace.address))
+                {
+                    let return_data = if !trace.success {
+                        let revert_msg =
+                            self.revert_decoder.decode(&trace.output, Some(trace.status), None);


this will only work for local contracts right? I'm wondering if there's some common bytecode pattern that is being reached when contract is called with such wrong calldata which we could catch and make this logic work for any contract for which we don't know abi

yes, exactly this would only provide insights if we know the abi beforehand

i guess a generic approach would be to perform some sort bytecode analysis and identify when the PC doesn't do any jumps on the function dispatcher and ends up reverting (idk if this vyper uses the same approach as solidity though)

0xrusowsky · 2025-05-20T23:11:29Z

@klkvr i've addressed all the feedback that u left:

improved docs
made things more restrictive by enforcing "empty revert data" as a condition
injected the revert reason into the inspector --> to do this, i had to move the fn call_end() logic into fn step()

lmk if i should fix something else 🙂

PS: as discussed, we'd leave the trace decoder improvement (where we would analyze that there are no matches in the fn dispatcher) for another PR

…ert-diagnostic-inspector

klkvr

lgtm, only have small nits

crates/evm/evm/src/inspectors/revert_diagnostic.rs

klkvr · 2025-05-21T01:25:05Z

crates/evm/evm/src/inspectors/revert_diagnostic.rs

+        if let Ok(state) = ctx.journal().code(target) {
+            if state.is_empty() && !inputs.input.is_empty() {
+                self.non_contract_call = Some((target, inputs.scheme, ctx.journal().depth()));
+            }
+        }


note: pattern CALL -> REVERT actually means that Solidity was not able to decode outputs which might happen even when contract has code but reported with invalid abi encoding like in e.g common case of ERC20 transfer not returning bool for some tokens. Ideally we should also be able to catch this case but we can track this separately

i'll open a new issue for that one and the unsupported selector.

just to be certain that i don't miss anything:

missmatch between bytecode and assumed abi: we can catch it cause there is a successful call, followed by a revert at the same depth where the call took place --> to avoid false positives, we should know check the returndata validation logic, but that requires careful inspection of the opcodes following the call. any hints on what u'd do exactly?

call to non supported fn selector: we can catch it (at least assuming that the target is a solidity/vyper contract) if, when a call takes places, there is a revert without any previous positive JUMPI, as that would mean that the PC walks through the whole fn dispatcher without finding a match --> using the traces decoder is limited to local contracts, but will never throw false positives. with this logic we could do everything with the inspector alone, which i like. however, do you think the approach is sound enough?

crates/evm/evm/src/inspectors/revert_diagnostic.rs

0xrusowsky · 2025-05-21T11:21:53Z

@grandizzy finally ready, with @klkvr last round of feedback incorporated!

grandizzy

thank you, good to send it from my pov! We should make some comparison with the project we use to have as a baseline (see #10183 (comment)) to be aware of penalty it introduce when forge test with traces.

@klkvr @zerosnacks @DaniPopes last pass through before merge please, thank you!

zerosnacks

lgtm!

chore: revert diagnostic inspector

3439f21

github-project-automation bot added this to Foundry May 6, 2025

0xrusowsky added 2 commits May 6, 2025 14:07

style: clippy + fmt

bdff692

style: more readable code + docs

381a47a

0xrusowsky linked an issue May 6, 2025 that may be closed by this pull request

Better error reporting when interacting with undeployed contracts #4141

Closed

grandizzy reviewed May 7, 2025

View reviewed changes

klkvr reviewed May 14, 2025

View reviewed changes

fix: track call stack depth + EXTCODESIZE checks

337c358

0xrusowsky requested review from grandizzy and klkvr May 15, 2025 15:04

0xrusowsky and others added 3 commits May 16, 2025 12:22

chore(traces decoder): non-supported fn selector call

68a4577

disable diagnostic revert when verbosity < '-vvv'

8c571d5

Merge branch 'master' into chore/revert-diagnostic-inspector

bbb18a8

0xrusowsky marked this pull request as ready for review May 16, 2025 10:48

0xrusowsky requested review from DaniPopes, mattsse, yash-atreya and zerosnacks as code owners May 16, 2025 10:48

grandizzy reviewed May 16, 2025

View reviewed changes

crates/evm/evm/src/inspectors/revert_diagnostic.rs Outdated Show resolved Hide resolved

0xrusowsky added 2 commits May 19, 2025 08:12

fix: do not warm address

89e4fe8

fix: call inspector

0ab8905

grandizzy reviewed May 19, 2025

View reviewed changes

crates/forge/src/multi_runner.rs Outdated Show resolved Hide resolved

0xrusowsky added 2 commits May 19, 2025 09:21

fix: merge conflicts

832c2ac

fix: config revert diag with fn tracing()

959c713

grandizzy changed the title ~~chore(forge): revert diagnostic inspector~~ feat(forge): revert diagnostic inspector May 19, 2025

grandizzy added T-feature Type: feature C-forge Command: forge labels May 19, 2025

grandizzy self-requested a review May 19, 2025 08:57

grandizzy previously approved these changes May 19, 2025

View reviewed changes

zerosnacks previously approved these changes May 19, 2025

View reviewed changes

klkvr requested changes May 20, 2025

View reviewed changes

improve docs + make diagnostics more restrictive

e0c5f9e

0xrusowsky dismissed stale reviews from zerosnacks and grandizzy via e0c5f9e May 20, 2025 21:24

0xrusowsky added 2 commits May 20, 2025 17:54

inject revert reason directly into interpreter

af5e1cc

style: clippy + fmt

656fb2d

0xrusowsky requested a review from klkvr May 20, 2025 23:05

0xrusowsky added 2 commits May 20, 2025 18:37

Merge branch 'master' of github.com:foundry-rs/foundry into chore/rev…

2a9d9ca

…ert-diagnostic-inspector

integrate new revm version

dfea9f0

klkvr previously approved these changes May 21, 2025

View reviewed changes

style: nits

4a606ce

0xrusowsky dismissed klkvr’s stale review via 4a606ce May 21, 2025 10:36

Merge branch 'master' into chore/revert-diagnostic-inspector

ced741e

grandizzy requested review from grandizzy, klkvr and zerosnacks May 21, 2025 11:22

grandizzy approved these changes May 21, 2025

View reviewed changes

zerosnacks approved these changes May 21, 2025

View reviewed changes

klkvr approved these changes May 22, 2025

View reviewed changes

klkvr merged commit 4332dc4 into foundry-rs:master May 22, 2025
22 checks passed

github-project-automation bot moved this to Done in Foundry May 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(forge): revert diagnostic inspector #10446

feat(forge): revert diagnostic inspector #10446

0xrusowsky commented May 6, 2025 •

edited

Loading

grandizzy left a comment

grandizzy May 7, 2025

0xrusowsky May 7, 2025

klkvr May 14, 2025

klkvr May 14, 2025

0xrusowsky commented May 15, 2025

0xrusowsky commented May 16, 2025

grandizzy left a comment

grandizzy left a comment

zerosnacks left a comment

0xrusowsky commented May 19, 2025

klkvr left a comment

klkvr May 20, 2025

0xrusowsky May 20, 2025

0xrusowsky commented May 20, 2025 •

edited

Loading

klkvr left a comment

klkvr May 21, 2025

0xrusowsky May 21, 2025 •

edited

Loading

0xrusowsky commented May 21, 2025

grandizzy left a comment

zerosnacks left a comment

feat(forge): revert diagnostic inspector #10446

feat(forge): revert diagnostic inspector #10446

Conversation

0xrusowsky commented May 6, 2025 • edited Loading

closes

design

output

benchmarks forge test -vvvv

grandizzy left a comment

Choose a reason for hiding this comment

grandizzy May 7, 2025

Choose a reason for hiding this comment

0xrusowsky May 7, 2025

Choose a reason for hiding this comment

klkvr May 14, 2025

Choose a reason for hiding this comment

klkvr May 14, 2025

Choose a reason for hiding this comment

0xrusowsky commented May 15, 2025

0xrusowsky commented May 16, 2025

grandizzy left a comment

Choose a reason for hiding this comment

grandizzy left a comment

Choose a reason for hiding this comment

zerosnacks left a comment

Choose a reason for hiding this comment

0xrusowsky commented May 19, 2025

klkvr left a comment

Choose a reason for hiding this comment

klkvr May 20, 2025

Choose a reason for hiding this comment

0xrusowsky May 20, 2025

Choose a reason for hiding this comment

0xrusowsky commented May 20, 2025 • edited Loading

klkvr left a comment

Choose a reason for hiding this comment

klkvr May 21, 2025

Choose a reason for hiding this comment

0xrusowsky May 21, 2025 • edited Loading

Choose a reason for hiding this comment

0xrusowsky commented May 21, 2025

grandizzy left a comment

Choose a reason for hiding this comment

zerosnacks left a comment

Choose a reason for hiding this comment

0xrusowsky commented May 6, 2025 •

edited

Loading

benchmarks `forge test -vvvv`

0xrusowsky commented May 20, 2025 •

edited

Loading

0xrusowsky May 21, 2025 •

edited

Loading